ENG-1233 Group functionality testing #678
Conversation
|
Updates to Preview Branch (eng-1233-group-functionality-testing) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
changed the base branch from
main
to
eng-1238-group-creation-function
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughTwo new test files introduce group-based content access testing. A Gherkin feature file defines test scenarios for creating groups, adding spaces to groups, and verifying content visibility permissions. Step definitions implement corresponding test steps and enhance database setup/teardown logic to handle group membership cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/database/features/step-definitions/stepdefs.ts:
- Around line 67-73: The Set of group IDs built from r3.data may include nulls,
so before calling client.auth.admin.deleteUser you should filter out
null/undefined values: when creating groupIds from (r3.data || []), only map and
add entries where group_id !== null && group_id !== undefined (or use
.filter(({group_id})=>group_id != null) before mapping), then iterate that
filtered set and call client.auth.admin.deleteUser(id) as before and assert
ur.error is null.
- Around line 406-419: The handler for When("user of space {word} creates group
{word}") accesses response.data!.group_id without verifying response.data; add a
null/undefined check on response.data (and on response.data.group_id) after the
invoke call and before assigning to localRefs so you don't use a non-null
assertion. If data is missing, assert.fail or throw a clear error (e.g.,
referencing response.error and response) so failures are explicit; update the
assignment to localRefs[name] only after confirming response.data.group_id
exists.
- Around line 74-79: The .not filter is using the string 'null' instead of the
JavaScript null which makes the Supabase/PostgREST query incorrect; update the
query call on client.from("PlatformAccount").select(...) to use
.not('dg_account', 'is', null) so the filter checks for actual NULLs, then keep
the subsequent loop that calls client.auth.admin.deleteUser(dg_account!) and the
assertions as-is.
🧹 Nitpick comments (1)
packages/database/features/step-definitions/stepdefs.ts (1)
430-441: Remove unused client1 variable.
client1is created on line 430 but never used. Onlyclient2is needed to query the member'sdg_account, and thenclient1is used again to insert the group membership. This works, but the initialclient1assignment on line 430 serves no purpose.♻️ Simplify by removing unused initial assignment
- const client1 = await getLoggedinDatabase(space1Id as number); const client2 = await getLoggedinDatabase(space2Id as number); const r1 = await client2.from("PlatformAccount").select("dg_account").eq("account_local_id", spaceAnonUserEmail("Roam", space2Id)).maybeSingle(); assert.equal(r1.error, null); const memberId = r1.data?.dg_account; assert(!!memberId); + const client1 = await getLoggedinDatabase(space1Id as number); const r2 = await client1.from("group_membership").insert({ group_id: groupId, member_id: memberId! }); assert.equal(r2.error, null);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/database/features/groupAccess.featurepackages/database/features/step-definitions/stepdefs.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
packages/database/features/step-definitions/stepdefs.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (2)
packages/database/features/groupAccess.feature (1)
1-36: LGTM! Well-structured test scenario.The feature file clearly defines the group content access workflow with appropriate acceptance criteria. The test scenario correctly validates that content remains isolated until explicitly published to a group via ContentAccess mapping, which aligns with the user story.
packages/database/features/step-definitions/stepdefs.ts (1)
407-407: Type change in localRefs - ensure downstream compatibility.The
localRefstype has been widened fromRecord<string, number>toRecord<string, number|string>to accommodate group IDs (UUIDs). Ensure that all existing code accessinglocalRefshandles this correctly, particularly code that assumes numeric values.#!/bin/bash # Description: Find all usages of localRefs to ensure they handle the union type correctly # Search for localRefs assignments and accesses rg -n "localRefs\[" packages/database/features/step-definitions/stepdefs.ts -C 2
| const r3 = await client.from("group_membership").select("group_id"); | ||
| assert.equal(r3.error, null); | ||
| const groupIds = new Set((r3.data || []).map(({group_id})=>group_id)); | ||
| for (const id of groupIds) { | ||
| const ur = await client.auth.admin.deleteUser(id); | ||
| assert.equal(ur.error, null); | ||
| } |
There was a problem hiding this comment.
Filter out null group_ids before deletion.
If group_id can be null in the group_membership table, this code will attempt to delete a user with id = null, which could cause an error or unexpected behavior.
🛡️ Proposed fix to filter nulls
const r3 = await client.from("group_membership").select("group_id");
assert.equal(r3.error, null);
- const groupIds = new Set((r3.data || []).map(({group_id})=>group_id));
+ const groupIds = new Set((r3.data || []).map(({group_id})=>group_id).filter(id => id !== null));
for (const id of groupIds) {
const ur = await client.auth.admin.deleteUser(id);
assert.equal(ur.error, null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const r3 = await client.from("group_membership").select("group_id"); | |
| assert.equal(r3.error, null); | |
| const groupIds = new Set((r3.data || []).map(({group_id})=>group_id)); | |
| for (const id of groupIds) { | |
| const ur = await client.auth.admin.deleteUser(id); | |
| assert.equal(ur.error, null); | |
| } | |
| const r3 = await client.from("group_membership").select("group_id"); | |
| assert.equal(r3.error, null); | |
| const groupIds = new Set((r3.data || []).map(({group_id})=>group_id).filter(id => id !== null)); | |
| for (const id of groupIds) { | |
| const ur = await client.auth.admin.deleteUser(id); | |
| assert.equal(ur.error, null); | |
| } |
🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 67 -
73, The Set of group IDs built from r3.data may include nulls, so before calling
client.auth.admin.deleteUser you should filter out null/undefined values: when
creating groupIds from (r3.data || []), only map and add entries where group_id
!== null && group_id !== undefined (or use .filter(({group_id})=>group_id !=
null) before mapping), then iterate that filtered set and call
client.auth.admin.deleteUser(id) as before and assert ur.error is null.
| const r2 = await client.from("PlatformAccount").select("dg_account").not('dg_account', 'is', 'null'); | ||
| assert.equal(r2.error, null); | ||
| for (const {dg_account} of r2.data || []) { | ||
| const r = await client.auth.admin.deleteUser(dg_account!); | ||
| assert.equal(r.error, null); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Supabase PostgREST .not() method syntax null values documentation
💡 Result:
Use the raw PostgREST filter syntax. To filter "not null" with Supabase's .not(), pass operator "is" and JavaScript/SDK null value, e.g.:
-
JavaScript / TypeScript:
supabase.from('table').select().not('column', 'is', null). [1] -
Other SDKs follow the same idea (e.g. Dart example). [2]
Notes/caveats:
- .not() expects PostgREST-style operators/values (e.g. use '(1,2)' for in, '{"a","b"}' for arrays). [1][2]
- There are known PostgREST edge-cases/issues around negating IS NULL (discussion in PostgREST repo). If you encounter parsing errors, check PostgREST version/issue tracker. [3]
Sources:
[1] Supabase JavaScript docs — not().
[2] Supabase Dart docs — not().
[3] PostgREST GitHub issue re: negating IS NULL.
Remove quotes around null in the .not() filter.
The syntax .not('dg_account', 'is', 'null') is incorrect. According to Supabase PostgREST documentation, the null value should be passed as a JavaScript null, not a string: .not('dg_account', 'is', null).
🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 74 -
79, The .not filter is using the string 'null' instead of the JavaScript null
which makes the Supabase/PostgREST query incorrect; update the query call on
client.from("PlatformAccount").select(...) to use .not('dg_account', 'is', null)
so the filter checks for actual NULLs, then keep the subsequent loop that calls
client.auth.admin.deleteUser(dg_account!) and the assertions as-is.
| When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{ | ||
| const localRefs = (world.localRefs || {}) as Record<string, number|string>; | ||
| const spaceId = localRefs[spaceName]; | ||
| if (spaceId === undefined) assert.fail("spaceId"); | ||
| const client = await getLoggedinDatabase(spaceId as number); | ||
| try{ | ||
| const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}}); | ||
| assert.equal(response.error, null); | ||
| localRefs[name] = response.data!.group_id; | ||
| } catch (error) { | ||
| console.error((error as any).actual); | ||
| throw error; | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add null check before accessing response.data.
Line 414 uses a non-null assertion response.data!.group_id without first verifying that response.data exists. While the assertion on line 413 checks for response.error, a successful response could still have a null or undefined data field.
🛡️ Proposed fix to add data validation
try{
const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}});
assert.equal(response.error, null);
- localRefs[name] = response.data!.group_id;
+ if (!response.data?.group_id) {
+ throw new Error("create-group response missing group_id");
+ }
+ localRefs[name] = response.data.group_id;
} catch (error) {
console.error((error as any).actual);
throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{ | |
| const localRefs = (world.localRefs || {}) as Record<string, number|string>; | |
| const spaceId = localRefs[spaceName]; | |
| if (spaceId === undefined) assert.fail("spaceId"); | |
| const client = await getLoggedinDatabase(spaceId as number); | |
| try{ | |
| const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}}); | |
| assert.equal(response.error, null); | |
| localRefs[name] = response.data!.group_id; | |
| } catch (error) { | |
| console.error((error as any).actual); | |
| throw error; | |
| } | |
| }) | |
| When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{ | |
| const localRefs = (world.localRefs || {}) as Record<string, number|string>; | |
| const spaceId = localRefs[spaceName]; | |
| if (spaceId === undefined) assert.fail("spaceId"); | |
| const client = await getLoggedinDatabase(spaceId as number); | |
| try{ | |
| const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}}); | |
| assert.equal(response.error, null); | |
| if (!response.data?.group_id) { | |
| throw new Error("create-group response missing group_id"); | |
| } | |
| localRefs[name] = response.data.group_id; | |
| } catch (error) { | |
| console.error((error as any).actual); | |
| throw error; | |
| } | |
| }) |
🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 406 -
419, The handler for When("user of space {word} creates group {word}") accesses
response.data!.group_id without verifying response.data; add a null/undefined
check on response.data (and on response.data.group_id) after the invoke call and
before assigning to localRefs so you don't use a non-null assertion. If data is
missing, assert.fail or throw a clear error (e.g., referencing response.error
and response) so failures are explicit; update the assignment to localRefs[name]
only after confirming response.data.group_id exists.
https://linear.app/discourse-graphs/issue/ENG-1233/group-functionality-testing
Verify that publishing content to a group makes it available to group members
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.